Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CAMEL-20250: resume after restart of Kinesis consumer #12462

Merged
merged 1 commit into from
Dec 19, 2023
Merged

CAMEL-20250: resume after restart of Kinesis consumer #12462

merged 1 commit into from
Dec 19, 2023

Conversation

abendt
Copy link
Contributor

@abendt abendt commented Dec 16, 2023 •
edited

Description

While trying to implement a resume strategy for a Kinesis consumer I ran into some issues that prevented me from implementing a strategy:

  • the shardId is not available in the exchange. The shardId is required to properly configure the GetShardIteratorRequest in the resume scenario (AWS doc)
  • the KinesisResumeAdapter API does not allow to pass in the shardId.
  • When consuming multiple shards the KinesisResumeAdapter will potentially be used by multiple threads concurrently. With the current API, this could cause concurrency issues

This PR proposes changes for the mentioned problems and provides a KinesisResumeStrategy implementation.

Target

  • I checked that the commit is targeting the correct branch (note that Camel 3 uses camel-3.x, whereas Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

https://issues.apache.org/jira/browse/CAMEL-20250

Mailing list:

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.
  • I have run mvn clean install -DskipTests locally and I have committed all auto-generated changes

@github-actions GitHub Actions
Copy link
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟

🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run

  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot.

  • You can label PRs using build-all, build-dependents, skip-tests and test-dependents to fine-tune the checks executed by this PR.

  • Build and test logs are available in the Summary page. Only Apache Camel committers have access to the summary.

  • ⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

oscerd
oscerd approved these changes Dec 16, 2023
View reviewed changes
Copy link
Contributor

@oscerd oscerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but @orpiske should take a look too

@davsclaus
Copy link
Contributor

There are uncommitted changes
HEAD detached at pull/12462/merge
Untracked files:
(use "git add ..." to include in what will be committed)
components/camel-aws/camel-aws2-kinesis/src/generated/resources/META-INF/services/org/apache/camel/kinesis-resume-strategy

@davsclaus
Copy link
Contributor

The PR has some changes not committed so the PR cannot automatic be tested, can you take a look and add those files in this PR

@orpiske
Copy link
Contributor

orpiske commented Dec 17, 2023

Thanks for your contribution. I am on PTO... I will take a look tomorrow.

@abendt
Copy link
Contributor Author

abendt commented Dec 17, 2023

I have committed the missing files

@orpiske orpiske changed the title resume after restart of Kinesis consumer CAMEL-20250: resume after restart of Kinesis consumer Dec 18, 2023
orpiske
orpiske requested changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@orpiske orpiske left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!!!

Overall it looks good to me. As this seems to cause the Kinesis resume to not work correctly, I created the ticket CAMEL-20250 so we can track it.

Once merged, we also need to review the example.

For now, I am only putting as "request changes" in order to ask for the commit message to be adjusted to mention the ticket I created (i.e.: just adjust the commit message to be something like "CAMEL-20250: the description of the changes") .

@abendt
Copy link
Contributor Author

abendt commented Dec 18, 2023

@orpiske i have added the ticket# to the commit message

@abendt abendt requested a review from orpiske December 18, 2023 13:35
orpiske
orpiske approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@orpiske orpiske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @abendt !

@abendt abendt marked this pull request as ready for review December 18, 2023 14:05
@oscerd oscerd merged commit a65705d into apache:main Dec 19, 2023
3 checks passed
ryan-highley pushed a commit to ryan-highley/camel that referenced this pull request Apr 8, 2024
@abendt @ryan-highley
CAMEL-20250: AWS Kinesis consumer - add KinesisResumeStrategy (apache…
4f3ccd3 
…#12462)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davsclaus davsclaus davsclaus approved these changes

@orpiske orpiske orpiske approved these changes

@oscerd oscerd oscerd approved these changes

Assignees
No one assigned
Labels
camel-4 components components-aws
Projects
None yet
Milestone
No milestone
4 participants
@abendt @davsclaus @orpiske @oscerd